-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
fix: prevent duplicate title tags in document head #5920
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
This fix addresses invalid HTML caused by multiple <title> elements when users manually set titles before router head management kicks in. Changes: - Modify Asset.tsx to use document.title API on client side - Automatically remove conflicting title tags on first update - Maintain normal SSR behavior for SEO compatibility - No API changes - fully backward compatible Fixes duplicate title tags while keeping the developer experience simple. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/react-router/src/Asset.tsx (1)
42-48: Consider stronger typing for attrs.The
attrsparameter is typed asRecord<string, any>, which is permissive. Since it's only used in the server-side render (line 75) and title elements support a limited set of attributes, consider using a more specific type:interface TitleAttrs { id?: string className?: string lang?: string dir?: 'ltr' | 'rtl' | 'auto' // Add other valid HTML title attributes as needed } function Title({ attrs, children, }: { attrs?: TitleAttrs children?: string }) {This provides better type safety and autocomplete for consumers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/react-router/src/Asset.tsx(2 hunks)
🔇 Additional comments (3)
packages/react-router/src/Asset.tsx (3)
19-19: LGTM: Clean delegation to Title component.The refactor to delegate title rendering to a dedicated component is a good separation of concerns and aligns with the PR objective to prevent duplicate title tags.
68-78: SSR/CSR split is correctly implemented.Verification confirms the implementation is sound:
- Server-side: Renders
<title>withsuppressHydrationWarning(line 75)- Client-side: Returns
null(line 70) to prevent rendering the title tag- Hydration handling:
useEffect(lines 51-66) runs after hydration completes and managesdocument.titledirectly, removing any existing title tags exactly once via thetitleControlledflag- Pattern consistency: Matches the Script component and other Asset components throughout the codebase
The intentional server-to-client mismatch (title tag → null) is properly suppressed and handled by the
useEffect, so no hydration warnings will occur and no flicker will happen during the SSR → hydration → client transition.
51-66: Design decisions confirmed: title persistence, attrs handling, and race condition guards are intentional.The code's behavior is by design:
No cleanup function is correct. The title persists after unmount because routes change without destroying the component—the next route's title update resets it. This is the intended pattern for client-side routing.
attrs only apply to server render. On the client,
document.titleis a string property, not a DOM element, so attrs likelang,dir,iddon't apply. The server renders<title {...attrs}>for SSR, and the client managesdocument.titledirectly. This differs from theScriptcomponent (which uses attrs on both client and server), but that's because scripts are DOM elements on both sides.titleControlled guards against duplicate removals. The module-level flag prevents the cleanup code (lines 55-59) from running more than once globally. React serializes effects within a render, and across navigation, the flag persists, so only the first Title component removes existing titles. This is safe in practice.
No issues found. The implementation matches the intended design pattern for title management in a client-side router.
| // Track if we've taken control of the title | ||
| let titleControlled = false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Module-level state causes lifecycle and isolation issues.
The titleControlled flag persists for the module's lifetime and is shared across all Title instances, which breaks in several scenarios:
- Testing: The flag isn't reset between test runs, causing test pollution.
- HMR: During development hot reloads, the flag remains true, preventing cleanup logic from running.
- Multiple routers: If the app uses nested routers or multiple router instances, they incorrectly share this flag.
- Microfrontends: Different apps on the same page will interfere with each other.
Consider one of these alternatives:
Option 1: Track in Router context (preferred if router has instance state):
// In Router initialization
this.titleControlled = false
// In Title component
const router = useRouter()
if (!router.titleControlled) {
// cleanup logic
router.titleControlled = true
}Option 2: Use a ref per component (if only one Title should ever be active):
-let titleControlled = false
-
function Title({ attrs, children }: { attrs?: Record<string, any>; children?: string }) {
const router = useRouter()
+ const hasCleanedUp = React.useRef(false)
React.useEffect(() => {
if (typeof children === 'string') {
- if (!titleControlled) {
+ if (!hasCleanedUp.current) {
const existingTitles = Array.from(document.querySelectorAll('title'))
existingTitles.forEach(titleEl => titleEl.remove())
- titleControlled = true
+ hasCleanedUp.current = true
}
document.title = children
}
}, [children])Note: Option 2 still has issues if multiple Title components mount simultaneously, but it's better isolated. Option 1 is the robust solution.
🤖 Prompt for AI Agents
In packages/react-router/src/Asset.tsx around lines 39-40, the module-level let
titleControlled = false creates shared state across all Title instances causing
test/HMR/multi-router/microfrontend issues; change it to instance-scoped state
by removing the module variable and tracking title control on the router
instance (preferred) — add a titleControlled boolean to the Router
initialization and read/update it from the Title component via the router
context (use router.titleControlled to decide/perform cleanup and set it true),
or if router changes are not possible, replace the module variable with a
per-component ref so each Title manages its own control state; ensure tests and
HMR no longer rely on module-level persistence.
PR Description:
🐛 Problem & User Need
The Original Need
I want to display meaningful page titles immediately during route loading, before
head()executes. Currently, there's a title gap because:This creates poor UX with blank or stale titles during navigation.
Current Workaround & Issue
Based on existing APIs, the only way to set immediate titles is in
beforeLoad:This causes duplicate title tags:
The result is invalid HTML with multiple
<title>elements, causing SEO and browser issues.✅ Solution
This PR enables the workaround mentioned above by fixing the duplicate title issue. Now developers can safely use
beforeLoadto set immediate titles without worrying about invalid HTML.The fix modifies title handling in
Asset.tsxto:<title>tagsdocument.titleinstead of rendering additional<title>elements<title>tags normally on the server for SEOResult: Clean HTML
Technical Implementation
Client-side behavior:
<title>DOM elementsdocument.titleAPI<title>tags are renderedServer-side behavior:
<title>tag rendering for SSR/SEO🔧 Implementation Details
File Changed:
packages/react-router/src/Asset.tsxBefore:
After:
📝 Usage & Developer Experience
Before This Fix
Developers faced a dilemma:
head()→ good HTML, but title gaps during loadingbeforeLoad+head()→ immediate titles, but invalid HTML with duplicatesAfter This Fix
Developers can now safely use the
beforeLoadpattern without HTML validity concerns:Summary: This PR fixes duplicate title tags by implementing clean title management that automatically handles conflicts while maintaining full backward compatibility and SSR support.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.